-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Reactive @KafkaListener [DO NOT MERGE] #1123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple concerns from me.
samples/sample-04/kafka-4-reactor/src/main/java/com/example/Kafka4ReactorApplication.java
Show resolved
Hide resolved
class Listener { | ||
|
||
@KafkaListener(topics = "skReactorTopic") | ||
public Disposable listen(Flux<ReceiverRecord<String, String>> flux) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the return type must be Mono<Void>
to postpone a subscription to the container.
Similar way what we have with WebFlux and RSocket implementations in Spring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with Artem that it would be better to return Mono<Void>
here
We would appreciate to have your feedback on this one since you are familiar with Reactor Kafka. Thank you in advance! |
|
||
@Override | ||
public void stop() { | ||
if (this.disposable != null && !this.disposable.isDisposed()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- no need to check for
isDisposed
, it should be idempotent - hint: you can pre-initialize the field with something like
static final Disposable DISPOSED = Disposable.disposed()
to avoid the null checks
acddf0c
to
2649b72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple concerns so far.
import org.springframework.util.StringUtils; | ||
import org.springframework.validation.Validator; | ||
|
||
import reactor.core.publisher.Flux; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this Reactor becomes as not a optional
dependency any more...
Is it really what we are aiming now ?
public void start() { | ||
ReceiverOptions<?, ?> options = ReceiverOptions.create(this.configs) | ||
.subscription(this.topics); | ||
Flux<?> flux = KafkaReceiver.create(options).receive(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything wrong with using ReactiveKafkaConsumerTemplate
instead?
Closing for now. |
Hi, has there been issues with the implementation so far - or has it simply been de-scoped for now? |
We just haven't had the bandwidth to work on it; we have also discovered some problems with the reactor-kafka project with respect to producer fencing when transactions are being used. That issue needs to be addressed over there; I hope to find some time to look at it soon. |
Closing for now. |
@garyrussell can you update the progress about this subject? I have some questions which I can't find answers to:
|
|
I understand. Thank you! |
Hi @garyrussell, I'm reading this thread again and I wonder if after the PR will be merged reactor/reactor-kafka#258 you plan of reactivating this current PR for a reactive |
I don't see any "code reducing" here. Reactor Kafka is doing the same work as the listener container; the goal being to get records from the broker and pass them to user code. The listener container and consumer event loop are effectively doing the same thing. KafkaReceiver.create(ro)
.receive()
.publishOn(Schedulers.single(), 1)
.doOnNext(record -> {
processIt(record);
})
...
.subscribe(); I am struggling to see the value add; a concrete example might help. |
@garyrussell the above code work perfect as you suggested and working fine for me. But can you help with the example where reactor-kafka can concurrently subscribe from multiple partitions of the topic concurrently in the same service instance. I don't want to leave reactor kafka just in absence of this feature. May be I am not aware. Please help. |
@harishvashistha It is not clear what you mean; you can subscribe to multiple topics in the |
@garyrussell I mean, I have this code which returns ServerSentEvent to SSE API over HTTP by fetching records from kafka. But this creates a single consumer to kafka topic with 20 partition:
As you said above, this kafkaReceiver.receive() just creates a single kafka consumer for all the partitions of the kafka topic. But how can I create multiple kafka consumers reading data from multiple partitions parallely as per my requirement. As I have to return this Flux of ServerSentEvent to the caller client over SSE API? I have followed this thread as well shared in https://stackoverflow.com/questions/69891782/multi-threading-on-kafka-send-in-spring-reactor-kafka . But not sure how to use this code to return Flux of ServerSentEvent to calling API:
|
You would need to create multiple receivers and merge their results into a single flux. |
@garyrussell are you hinting that something like this would be required?:
As this is not returning any data to SSE channel at all.
|
Sorry; I am not a reactor expert; you'd be better off asking somewhere like stack overflow. That said, you seem to be discarding the new flux created by
Looks like it should be
|
Initial PoC only.